-
Notifications
You must be signed in to change notification settings - Fork 341
Clean up docs workflows #3231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up docs workflows #3231
Conversation
Don't need to run on updates of docs-update-dependency-common.yml because that's already being checked by docs-update-doc-builder.
No actual difference in container.
74cd8fa to
d5b7f1e
Compare
# Conflicts: # .github/workflows/docs-omnibus.yml # doc/testing.sh
ekluzek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samsrabin thanks so much for your work here. This is a lot of improvement, breaking things up to isolate complexity, and formalizing of the testing for docs, which is really great to bring in.
The reason I put the request changes, button is that you will need to trap for errors in the bash testing. I think a lot of the tests will continue with errors, or at least not have the clearest kind of error handling for developers to see what to do to fix a test. So I see that as critical to make sure is working well.
It also made me realize we need to think about how we should do our testing for bash. So I'll open a discussion on that, and we'll talk about it at a future Thursday meeting. Most of that is outside the scope here, but a good thing to be thinking about.
Some small things I suggest:
- Change the name of doc/testing to doc/test
- Change "Yep!" to something like ... "Successfully compared that the two methods give identical results!"
- Consider changing the filenames with a dash option in the name
- Ask for a few comments in a few places
I also wonder if more things could be added into the common test part, but that can be tried later. This gives a structure to facilitate doing that sort of thing, so doesn't need to happen here.
Finally, what are the use cases for supporting the three build methods: makefile, ctsm_pylib, and doc-builder? Are all important to maintain? Could we drop any of them? If not this testing will be critical in order to support all of them. And we can assess this question outside of this PR, and do that later, as we need to think about this and discuss before we do anything about it.
ekluzek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking as approve now. I still have a few recommendations to do, but they are mostly in the space of adding comments.
I was concerned about the lack of error trapping, but the use of "set -e" will suffice for now. Longer term we should move to using error trapping for at least assert type testing. This allows for nice messaging on the problem and for the test script to continue afterwards.
Description of changes
Breaks up the "omnibus docs test" GitHub Workflow to minimize the number of workflow runs. Also solves problem where people were getting test failures on their forks.
Specific notes
Contributors other than yourself, if any: None
CTSM Issues Fixed: None
Are answers expected to change (and if so in what way)? No
Any User Interface Changes (namelist or namelist defaults changes)? No
Does this create a need to change or add documentation? Did you do so? No
Testing performed, if any:
testing.sh